Drop `Deref` indirection for most trait parameters #4311

valentinewallace · 2026-01-13T19:16:02Z

Reduces generics and verbosity across the codebase, should provide equivalent behavior.

Inspired by #4309 (comment). Basically, instead of Deref indirection we instead impl<T: Trait, D: Deref<Target = Trait>> Trait for D for all our traits.

The majority of the codebase's traits are done in this PR, but we do skip a good amount still. In a lot of cases traits are skipped because of compiler complaints about (potential-) conflicting trait implementations, but also ran into some lifetime issues and some c_bindings issues.

ldk-reviews-bot · 2026-01-13T19:16:05Z

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

valentinewallace · 2026-01-13T19:16:30Z

If this looks good and works for language bindings, I/claude can do it for all the other traits. I feel like this is a solid improvement.

valentinewallace · 2026-01-13T19:17:01Z

@TheBlueMatt would appreciate conceptual review to make sure this works for the language bindings.

valentinewallace · 2026-01-13T19:19:56Z

Actually, it looks like this might not work for 1.75 (associated type bounds are unstable). Not sure if we plan on bumping that anytime soon?

TheBlueMatt · 2026-01-13T20:32:05Z

@TheBlueMatt would appreciate conceptual review to make sure this works for the language bindings.

It means updating the generation logic to support it, but it certainly isn't directly problematic.

Actually, it looks like this might not work for 1.75 (associated type bounds are unstable). Not sure if we plan on bumping that anytime soon?

You can write it as impl<T: BroadcasterInterface, B: Deref<Target = T>> BroadcasterInterface for B { instead of impl<B: Deref<Target: BroadcasterInterface>> BroadcasterInterface for B {.

TheBlueMatt · 2026-01-13T20:32:28Z

Conceptually tho yea imo we should do this. Its "the right way" to do it in rust.

lightning/src/chain/chaininterface.rs

codecov · 2026-01-13T23:55:54Z

Codecov Report

❌ Patch coverage is 81.62912% with 212 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.01%. Comparing base (9df0280) to head (4c0875a).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/msgs.rs	35.05%	126 Missing ⚠️
lightning/src/util/persist.rs	45.94%	20 Missing ⚠️
lightning/src/ln/channel.rs	93.91%	14 Missing ⚠️
lightning/src/ln/peer_handler.rs	70.58%	9 Missing and 1 partial ⚠️
lightning-background-processor/src/lib.rs	68.18%	7 Missing ⚠️
lightning/src/chain/channelmonitor.rs	91.02%	7 Missing ⚠️
lightning/src/util/anchor_channel_reserves.rs	0.00%	6 Missing ⚠️
lightning/src/chain/chainmonitor.rs	77.77%	4 Missing ⚠️
lightning/src/onion_message/messenger.rs	92.00%	4 Missing ⚠️
lightning-net-tokio/src/lib.rs	0.00%	3 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4311      +/-   ##
==========================================
- Coverage   86.11%   86.01%   -0.10%     
==========================================
  Files         156      156              
  Lines      102674   102479     -195     
  Branches   102674   102479     -195     
==========================================
- Hits        88415    88151     -264     
- Misses      11764    11833      +69     
  Partials     2495     2495

Flag	Coverage Δ
tests	`86.01% <81.62%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

Note that you'll need to bump lightning-liquidity's version to 0.3.0+git to get the SemVer checks to pass, as it's an API-breaking change.

joostjager · 2026-01-15T18:42:15Z

Is the Logger a candidate too for this?

valentinewallace · 2026-01-15T18:49:01Z

Is the Logger a candidate too for this?

Yes, I will un-draft the PR once all the traits are complete :)

ldk-reviews-bot · 2026-01-15T19:18:08Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager · 2026-01-15T22:26:38Z

lightning/src/ln/msgs.rs

 	fn message_received(&self);
 }

+impl<T: ChannelMessageHandler + ?Sized, C: Deref<Target = T>> ChannelMessageHandler for C {


Is this really better than using Arc instead of Deref, and accepting that for no-std the API might look slightly different?

There aren't that many no-std projects.

Yeah maybe we should resolve this here #4309 (comment) before moving forward with this direction? Is it still difficult to add new trait params now that we have Claude?

Ask Claude to add a logger to ChannelMonitor, and then report back 😂

valentinewallace · 2026-01-20T17:26:20Z

@tnull do you have any idea if the LDK Node API CI failure is easy to fix on the LDK Node end? Otherwise I can drop the KVStore update commit.

TheBlueMatt · 2026-01-20T19:18:34Z

@tnull do you have any idea if the LDK Node API CI failure is easy to fix on the LDK Node end? Otherwise I can drop the KVStore update commit.

It is now the responsibility of rust-lightning PR authors to go fix ldk-node if your PR breaks it :p

valentinewallace · 2026-01-20T19:24:26Z

@tnull do you have any idea if the LDK Node API CI failure is easy to fix on the LDK Node end? Otherwise I can drop the KVStore update commit.

It is now the responsibility of rust-lightning PR authors to go fix ldk-node if your PR breaks it :p

I'm aware, but I'm not sure if it's something that should be fixed or whether the new rust-lightning API isn't good. Thought he might know off the top of his head :p

TheBlueMatt · 2026-01-20T19:31:20Z

Fair enough. It does look like its just a conflict cause there's a deref-bounded auto-impl, which shouldn't be crazy to fix. Would have to go actually try to be sure though.

jkczyz

Generally, LGTM. Will let @tnull comment on whether there's any perceived issues for downstream users.

TheBlueMatt · 2026-01-27T20:20:33Z

FWIW, I'd marginally prefer this wait until after #4342 because I want to backport the changes there to 0.2 and it has a bunch of the Deref stuff that would mean rewriting it in the backport which I always hate doing.

tnull

LGTM

I have no real opinion either way whether this refactor is worth the churn, but should work. Also confirmed that LDK Node tests and builds. So fine by me^^

Useful for upcoming commits where we otherwise break SemVer checks by changing the ALiquidityManager associated types.

lightning/src/routing/utxo.rs

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Unfortunately the same improvement can't be made for KVStoreSync and Persist, due to the blanket implementation where all KVStoreSync traits implement Persist resulting in conflicting implementations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. This could be split into multiple commits (e.g. one for OnionMessenger message handler types, one for PeerManager message handler types) but it would require adding a new IgnoringOnionMessageHandler type for the messenger handlers, due to the IgnoringMessageHandler's Deref implementation conflicting otherwise. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

valentinewallace requested a review from TheBlueMatt January 13, 2026 19:17

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 1c6cf54 to 584fec7 Compare January 13, 2026 22:34

valentinewallace commented Jan 13, 2026

View reviewed changes

lightning/src/chain/chaininterface.rs Show resolved Hide resolved

tnull reviewed Jan 14, 2026

View reviewed changes

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 584fec7 to ad7aaa5 Compare January 15, 2026 00:18

valentinewallace added this to Weekly Goals Jan 15, 2026

valentinewallace self-assigned this Jan 15, 2026

valentinewallace removed the request for review from TheBlueMatt January 15, 2026 19:26

valentinewallace force-pushed the 2026-01-avoid-derefs branch from ad7aaa5 to 028f1fb Compare January 15, 2026 21:49

joostjager reviewed Jan 15, 2026

View reviewed changes

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 028f1fb to 3630b91 Compare January 20, 2026 17:18

valentinewallace changed the title ~~Drop Deref indirection for ChannelManager, etc~~ Drop Deref indirection for most trait parameters Jan 20, 2026

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 3630b91 to 2ceb510 Compare January 20, 2026 17:22

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 2ceb510 to db87b32 Compare January 20, 2026 18:47

valentinewallace marked this pull request as ready for review January 20, 2026 18:48

ldk-reviews-bot requested a review from wpaulino January 20, 2026 18:49

jkczyz reviewed Jan 27, 2026

View reviewed changes

tnull reviewed Jan 29, 2026

View reviewed changes

valentinewallace mentioned this pull request Jan 29, 2026

Require WithContext log wrappers on OutboundPayments calls and pass payment hashes #4342

Merged

Bump lightning-liquidity version

3892eb0

Useful for upcoming commits where we otherwise break SemVer checks by changing the ALiquidityManager associated types.

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 3ab9476 to 1cf83d3 Compare January 29, 2026 19:00

valentinewallace added the weekly goal Someone wants to land this this week label Jan 29, 2026

valentinewallace requested review from jkczyz and tnull January 29, 2026 20:27

jkczyz reviewed Jan 29, 2026

View reviewed changes

lightning/src/routing/utxo.rs Outdated Show resolved Hide resolved

valentinewallace removed the request for review from tnull January 29, 2026 22:05

valentinewallace and others added 15 commits January 29, 2026 17:14

Drop Deref indirection for BroadcasterInterface

d82a0d7

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for EntropySource

db6eff9

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for NodeSigner

af869f5

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for FeeEstimator

337534b

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for Router

51fc0f6

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for MessageRouter

b646c2f

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for Logger

344859c

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for SignerProvider

695238c

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for chain::Watch

7df6a98

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for NodeIdLookup

fabbb00

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for chain::Filter

6bdd6b4

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for OutputSpender

6abc6be

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for UtxoLookup

4c0875a

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 1cf83d3 to 4c0875a Compare January 29, 2026 22:32

valentinewallace requested review from jkczyz and tnull January 29, 2026 22:33

jkczyz approved these changes Jan 29, 2026

View reviewed changes

Drop Deref indirection for most trait parameters #4311

Are you sure you want to change the base?

Drop Deref indirection for most trait parameters #4311

Conversation

valentinewallace commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace commented Jan 13, 2026

Uh oh!

valentinewallace commented Jan 13, 2026

Uh oh!

valentinewallace commented Jan 13, 2026

Uh oh!

TheBlueMatt commented Jan 13, 2026

Uh oh!

TheBlueMatt commented Jan 13, 2026

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jan 15, 2026

Uh oh!

valentinewallace commented Jan 15, 2026

Uh oh!

ldk-reviews-bot commented Jan 15, 2026

Uh oh!

joostjager Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Jan 20, 2026

Uh oh!

TheBlueMatt commented Jan 20, 2026

Uh oh!

valentinewallace commented Jan 20, 2026

Uh oh!

TheBlueMatt commented Jan 20, 2026

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jan 27, 2026

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Drop `Deref` indirection for most trait parameters #4311

Drop `Deref` indirection for most trait parameters #4311

valentinewallace commented Jan 13, 2026 •

edited

Loading

ldk-reviews-bot commented Jan 13, 2026 •

edited

Loading

codecov bot commented Jan 13, 2026 •

edited

Loading

tnull left a comment •

edited

Loading